Use `#[repr(transparent)]` newtype to hold FFI error codes #696

ecstatic-morse · 2018-10-06T18:33:08Z

Resolves #451.

This PR changes the definition of some foreign functions to return a bssl::ErrorCode instead of c::int.

Converting to a Result is now done through a conversion function rather than bssl::map_result. It also defines marker traits for the two widely used return code conventions in this library:

1 for success (used in most BoringSSL functions)
0 for success

I agree to license my contributions to each file under the terms given
at the top of each file I changed.

briansmith

In general I like the idea of using a #[repr(transparent)] type for the return types of the non-Rust code but I disagree with several of the details.

briansmith · 2018-10-07T06:01:43Z

src/aead/aes_gcm.rs

        GFp_aes_gcm_seal(ctx.as_ptr(), in_out.as_mut_ptr(), in_out.len(), tag,
                         nonce, ad.as_ptr(), ad.len())
-    })
+    }.into()


I don't find this as clear as the existing code but I guess people would learn it fast enough.

Would Result::from be a workable improvement?

Using Result::from everywhere is fine. You have to explicitly qualify it anyways when using the ? operator if the Ok type returned by the function is not ().

Another option is to add a method like into_result to ErrorCode which solves the type deduction problem but doesn't require wrapping the whole unsafe block in another set of parens.

briansmith · 2018-10-07T06:03:09Z

src/bssl.rs

+    use {c, error};
+
+    /// The meaning of the error code returned by a foreign function.
+    pub trait Semantics {


I don't see the advantage of factoring this out into a trait. One of the design principles of ring is to avoid unnecessary abstractions, and at least the way the code is currently, this abstraction isn't necessary.

Just two separate structs here then?

I had imagined that this would make it easy to share code later as features were added, but that's kind of the definition of overabstraction :)

Also, how do you feel about using Posix to mean "returns zero on failure". It was the best, shortest thing I could think of, but bssl::PosixResult reads a bit strangely.

briansmith · 2018-10-07T06:03:52Z

src/bssl.rs

+                1 => true,
+                c => {
+                    // Be pedantic in debug mode to catch some cases where function signatures are
+                    // wrongly defined.


What are examples of such wrongly-defined functions?

By wrongly defined I meant that an author adding a new extern declaration might mistakenly use the wrong return type (e.g. fn() -> bssl::ErrorCode for a function which returns a negative value on error).

In that case I would say "any cases where [whatever type name we decide on] might be misused."

briansmith · 2018-10-07T06:04:26Z

src/bssl.rs

+    #[must_use]
+    #[repr(transparent)]
+    #[derive(Debug, Clone, Copy)]
+    pub struct ErrorCode<S = BoringSsl> {


I don't think ErrorCode is a very good name for this boolean flag. I think Result would make more sense since BoringSSL's boolean results are basically Result<(), ()>s.

Agreed. I avoided it initially since modname::Result is usually a type alias for std::result::Result with the error type defined.

briansmith · 2018-10-07T06:57:23Z

src/bssl.rs

+    pub enum BoringSsl {}
+
+    impl Semantics for BoringSsl {
+        fn is_success(code: c::int) -> bool {


I would say, rather than defining an is_success() -> bool function, just inline this logic directly into the From implementation that does the conversion to Result<(), error::Unspecified>.

briansmith · 2018-10-07T07:22:28Z

src/aead/aes_gcm.rs

        fn GFp_AES_set_encrypt_key(key: *const u8, bits: usize,
-                                   aes_key: *mut AES_KEY) -> c::int;
+                                   aes_key: *mut AES_KEY) -> bssl::PosixStyleErrorCode;


Is this the only function like this you've found? I'd rather change the C code to use the regular boolean conventions and rename it, than add this complexity.

There are also system APIs being used which return 0 on success, but to my knowledge this is the only currently used function from BoringSSL which has that convention.

src/rsa/bigint.rs

ecstatic-morse · 2018-10-07T21:14:24Z

There's also a WIN32 function which returns one on success, but uses a c::win32::BOOLEAN instead of a c::int.

Based on your feedback as well as this new information, I'd like to make the following sets of changes:

Define a new module called c::result to make clear that the newly added types should only be used for ffi.
Define transparent newtypes c::result::BoringSsl, c::result::Posix and c::result::Win32Bool to handle various commonly used return types and conventions.
Publicly alias either c::Result or bssl::Result to c::result::BoringSsl.
Add size/alignment tests for the newtypes via the define_test_metrics macro.

I could also eliminate the POSIX-style and Win32Bool wrappers, change AES_set_encrypt_key to return a BoringSSL style result (or nothing at all), and add a #[must_use] declaration to the various system APIs which return an error code. Would you prefer this approach?

I was just perusing the list of good-first-bugss and saw this one. Sorry if this took more time for you to review than it would to have implemented it. Thanks for all the work you've done in the Rust community!

briansmith · 2018-10-07T22:51:01Z

For that one single Win32 function, why not just change the FFI declaration of the function to use function-level #[must_use]?:

        #[link_name = "SystemFunction036"]
        #[must_use]
        fn RtlGenRandom(random_buffer: *mut u8,
                        random_buffer_length: c::win32::ULONG)
                        -> c::win32::BOOLEAN;

I suggest we skip the Posix case for now as I think it's trickier and also I hope we can eliminate the use of "POSIX" semantics from non-Posix functions like that AES API (in a separate PR).

For the GFp/BoringSSL API, maybe we should "just" use #[must_use] instead too? What are the pros/cons from your perspective?

[ ] Publicly alias either c::Result or bssl::Result to c::result::BoringSsl.

I suggest we just skip this, regardless.

Add size/alignment tests for the newtypes via the define_test_metrics macro.

Sounds good to me, if we need the newtypes.

ecstatic-morse · 2018-10-07T23:15:13Z

Both approaches statically guarantee that error codes are being checked, so the issue is more about the conversion to Result currently done by bssl::map_result.

With the free function, the consumer must know the return code semantics at each call site, whereas with the newtype, the return code semantics must only be known at the declaration, and conversion can be done thoughtlessly via Result::from. Having to know the return code semantics at the call site isn't a large burden, especially if we normalize GFp_AES_set_encrypt_key to be consistent¸ but there will still be functions such as GFp_memcmp which return a c::int which doesn't indicate an error.

The presence or absence of #[must_use] shouldn't be used to indicate whether a function returns an error. GFp_memcmp could be marked #[must_use] as well so the compiler could warn when the result of a comparison is unused just as it does with a == b.

I understand the proliferation of return type wrappers being undesirable, especially given the efforts to oxidize ring, but I think it'd be good to provide one for the common case.

briansmith · 2018-10-07T23:30:42Z

OK, I think in this PR we should go with your approach for the GFp API, the approach I suggested above for the single Win32 function, and punt Posix to later. That should allow us to make progress here.

ecstatic-morse · 2018-10-07T23:34:34Z

Got it! I'll make the changes later today. Do you want the path to the type to be bssl::Result, c::Result or some spelling of c::result::BoringSsl`?

briansmith · 2018-10-09T21:07:40Z

appveyor.yml

@@ -11,7 +11,7 @@ platform:
 environment:
  matrix:
    - TOOLCHAIN_VERSION: 14.0
-      RUST: 1.26.0
+      RUST: 1.28.0


This should be done as a separate PR and we should be doing Rust 1.29.1.

Is there a reason this isn't just "stable" instead of an explicit version number?

briansmith · 2018-10-09T21:09:09Z

src/aead/aes_gcm.rs

@@ -172,6 +172,9 @@ mod tests {
    }

    extern "C" {
+        // AES_set_encrypt_key returns 0 on success, unlike most BoringSSL functions.
+        // https://github.com/google/boringssl/blob/28babde159253bfa9003a445242605806fff5f1f/include/openssl/aes.h#L83


Please remove the link to the BoringSSL code. If the comment being linked to is missing in ring then we should add the comment to ring.

I added a comment to ring. I'm going to fix this issue next, making it irrelevant.

briansmith · 2018-10-09T21:10:39Z

src/bssl.rs

-        1 => Ok(()),
-        _ => Err(error::Unspecified),
+/// A `c::int` returned from a foreign function containing **1** if the function was successful
+/// or **0** if an error occurred. This is the convention used by BoringSSL.


Let's remove the "This is the convention used by BoringSSL." I don't want to conflate ring with BoringSSL.

briansmith · 2018-10-09T21:11:27Z

src/bssl.rs

+/// or **0** if an error occurred. This is the convention used by BoringSSL.
+#[must_use]
+#[repr(transparent)]
+#[derive(Debug, Clone, Copy)]


Let's put #[derive(...)] first unless there's some reason not to. I usually sort these alphabetically when I remember to.

briansmith · 2018-10-09T21:12:20Z

src/bssl.rs

+            c => {
+                // Be pedantic in debug mode to catch any cases where functions are mistakenly
+                // declared to return a BoringSSL-style error code when they really have
+                // different semantics.


The comment is not necessary; let's just remove it.

briansmith · 2018-10-09T21:13:30Z

src/bssl.rs

+                // Be pedantic in debug mode to catch any cases where functions are mistakenly
+                // declared to return a BoringSSL-style error code when they really have
+                // different semantics.
+                debug_assert_eq!(c, 0, "BoringSSL functions should return 0 or 1");


I suggest "bssl::Result value must be be 0 or 1" instead.

briansmith · 2018-10-09T21:15:06Z

src/bssl.rs

+mod tests {
+    mod result {
+        use {bssl, c};
+        use std::mem;


use core::mem instead of std::mem. In general we use core instead of std whenever possible.

briansmith

Thanks! This looks good, modulo a few nits.

briansmith · 2018-10-13T01:01:47Z

@ecstatic-morse This looks great. Please squash this PR into a single commit that ends with the "I agree ..." statement. The commit message should be wrapped to ~72 characters.

I suggest this as the commit message (wrapped to ~72 characters):

Replace the `bssl::map_result` function with a new `bssl::Result` type.

I agree to ....

This commit introduces a `#[repr(transparent)]` newtype to represent the return type of foreign functions which return 1 on success and 0 on failure. This type is `#[must_use]`, so these return codes must be checked by the caller. It also adds `#[must_use]` to foreign functions which use a different convention to return an error. I agree to license my contributions to each file under the terms given at the top of each file I changed.

ecstatic-morse · 2018-10-15T21:23:29Z

Squashed and rebased.

briansmith · 2018-10-22T21:54:51Z

Thanks!

briansmith requested changes Oct 7, 2018

View reviewed changes

briansmith reviewed Oct 7, 2018

View reviewed changes

src/rsa/bigint.rs Show resolved Hide resolved

ecstatic-morse force-pushed the ffi-return-type branch from 0c8c066 to c217559 Compare October 8, 2018 08:01

briansmith reviewed Oct 9, 2018

View reviewed changes

briansmith requested changes Oct 9, 2018

View reviewed changes

ecstatic-morse force-pushed the ffi-return-type branch from 0834861 to 7ede26d Compare October 9, 2018 22:36

ecstatic-morse force-pushed the ffi-return-type branch from 7ede26d to d3ee0a0 Compare October 15, 2018 21:20

briansmith merged commit 408089d into briansmith:master Oct 22, 2018

ecstatic-morse deleted the ffi-return-type branch October 23, 2018 02:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use `#[repr(transparent)]` newtype to hold FFI error codes #696

Use `#[repr(transparent)]` newtype to hold FFI error codes #696

ecstatic-morse commented Oct 6, 2018 •

edited

Loading

briansmith left a comment

briansmith Oct 7, 2018

ecstatic-morse Oct 7, 2018

briansmith Oct 7, 2018

ecstatic-morse Oct 7, 2018

ecstatic-morse Oct 7, 2018 •

edited

Loading

briansmith Oct 7, 2018

ecstatic-morse Oct 7, 2018 •

edited

Loading

briansmith Oct 7, 2018 •

edited

Loading

briansmith Oct 7, 2018

ecstatic-morse Oct 7, 2018

briansmith Oct 7, 2018

briansmith Oct 7, 2018

ecstatic-morse Oct 7, 2018 •

edited

Loading

ecstatic-morse commented Oct 7, 2018 •

edited

Loading

briansmith commented Oct 7, 2018

ecstatic-morse commented Oct 7, 2018 •

edited

Loading

briansmith commented Oct 7, 2018

ecstatic-morse commented Oct 7, 2018

briansmith Oct 9, 2018

ecstatic-morse Oct 9, 2018

briansmith Oct 9, 2018

ecstatic-morse Oct 9, 2018

briansmith Oct 9, 2018

briansmith Oct 9, 2018

briansmith Oct 9, 2018

briansmith Oct 9, 2018

briansmith Oct 9, 2018

briansmith left a comment

briansmith commented Oct 13, 2018

ecstatic-morse commented Oct 15, 2018

briansmith commented Oct 22, 2018

Use #[repr(transparent)] newtype to hold FFI error codes #696

Use #[repr(transparent)] newtype to hold FFI error codes #696

Conversation

ecstatic-morse commented Oct 6, 2018 • edited Loading

briansmith left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Oct 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Oct 7, 2018 • edited Loading

Choose a reason for hiding this comment

briansmith Oct 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstatic-morse Oct 7, 2018 • edited Loading

Choose a reason for hiding this comment

ecstatic-morse commented Oct 7, 2018 • edited Loading

briansmith commented Oct 7, 2018

ecstatic-morse commented Oct 7, 2018 • edited Loading

briansmith commented Oct 7, 2018

ecstatic-morse commented Oct 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

briansmith left a comment

Choose a reason for hiding this comment

briansmith commented Oct 13, 2018

ecstatic-morse commented Oct 15, 2018

briansmith commented Oct 22, 2018

Use `#[repr(transparent)]` newtype to hold FFI error codes #696

Use `#[repr(transparent)]` newtype to hold FFI error codes #696

ecstatic-morse commented Oct 6, 2018 •

edited

Loading

ecstatic-morse Oct 7, 2018 •

edited

Loading

ecstatic-morse Oct 7, 2018 •

edited

Loading

briansmith Oct 7, 2018 •

edited

Loading

ecstatic-morse Oct 7, 2018 •

edited

Loading

ecstatic-morse commented Oct 7, 2018 •

edited

Loading

ecstatic-morse commented Oct 7, 2018 •

edited

Loading